Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet] Create new carousel component #1555

Merged
merged 11 commits into from
Nov 4, 2019
Merged

[Wallet] Create new carousel component #1555

merged 11 commits into from
Nov 4, 2019

Conversation

jmrossy
Copy link
Contributor

@jmrossy jmrossy commented Oct 31, 2019

Description

This is the first of many PRs that will build the new optional verification experience.

This adds a new carousel for user education:
https://www.figma.com/file/9lBnIzsABhO8oXE0d0c8Km/Onboarding-and-Verification?node-id=233%3A1559

Remove unused verify input screen
Create new screen for first stage of new verification designs

Tested

In app (android, still TODO iOS but the lib is cross-platform)

Related issues

Backwards compatibility

Yes

wa-carousel

return (
<SafeAreaView style={styles.container}>
<ScrollView contentContainerStyle={styles.scrollContainer}>
{/* <DevSkipButton nextScreen={Screens.WalletHome} /> */}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, DevSkipButton would not be visible in production builds. So, I'm curious about the reason to comment it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, it won't be. it was just a bit in the way as I was dev-ing this screen. Consider everything except the carousel still WIP

@codecov
Copy link

codecov bot commented Nov 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d4b8c38). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1555   +/-   ##
=========================================
  Coverage          ?   73.64%           
=========================================
  Files             ?      276           
  Lines             ?     7391           
  Branches          ?      950           
=========================================
  Hits              ?     5443           
  Misses            ?     1837           
  Partials          ?      111
Flag Coverage Δ
#mobile 73.64% <100%> (?)
Impacted Files Coverage Δ
packages/mobile/src/navigator/Screens.tsx 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4b8c38...c4738e5. Read the comment docs.

Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍

One thing in the screenshot is the shadow seems off compared to the figma design.

Comment on lines +28 to +29
<SafeAreaView style={styles.container}>
<ScrollView contentContainerStyle={styles.scrollContainer}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's only the ScrollView in this screen (i.e. nothing outside of it), it's nicer on iOS to have the SafeAreaView be inside the ScrollView so the content doesn't get clipped when overscrolling up or down.

@i1skn
Copy link
Contributor

i1skn commented Nov 4, 2019

Other than the comment I left regarding DevSkipButton it LGTM 👍 !

@jmrossy
Copy link
Contributor Author

jmrossy commented Nov 4, 2019

Nice! 👍

One thing in the screenshot is the shadow seems off compared to the figma design.

Yeah, shadows in React Native are very limited on android. I've improved it in the later PRs but still doesn't look perfect.

@jmrossy jmrossy merged commit e2f6485 into master Nov 4, 2019
@jmrossy jmrossy deleted the rossy/wa-carousel branch November 4, 2019 13:36
aaronmgdr added a commit that referenced this pull request Dec 5, 2019
* master: (73 commits)
  Fix Ethstats Image reference (#1577)
  EU Cookies Behavior Change (#1447)
  [verifier] Upgrade to RN 61 (#1572)
  [Wallet] Update link styles and Implement VerificationEducationScreen (#1565)
  [wallet] Added native phone picker (#1310)
  [Wallet] Set up new verification screen skeletons (#1563)
  Bump e2e test migrate numbers where needed (#1567)
  [Wallet] Create new carousel component (#1555)
  [Wallet] Protect Backup Key and Safeguards with PIN (#1556)
  Increase ganache gas limit (#1569)
  Re-work locked gold requirements for validators and groups (#1474)
  Fix e2e on CI (#1537)
  Allow a specified address to disable/enable the Exchange  (#1467)
  Avoid re-encrypting key files with yarn keys:encrypt command (#1560)
  Support protocol hotfixing (#613)
  Point e2e tests back (#1562)
  Refactor to Accounts.sol (#1392)
  Add selectIssuers Transaction (#1327)
  [Wallet] Get React Native Hot Reloading Working (#1551)
  Unify to prefix messages for signing (#1473)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new education carousel
6 participants